[clang][analyzer] Support ownership_{returns,takes} attributes
#98941
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-static-analyzer-1 @llvm/pr-subscribers-clang Author: Pavel Skripkin (pskrgag) ChangesAdd support for checking mismatched ownership_returns/ownership_takes attributes. Closes #76861 Full diff: https://github.com/llvm/llvm-project/pull/98941.diff 2 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index fe202c79ed620..821e0c8da5d2d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -103,14 +103,46 @@ using namespace std::placeholders;
namespace {
// Used to check correspondence between allocators and deallocators.
-enum AllocationFamily {
+enum AllocationFamilyKind {
AF_None,
AF_Malloc,
AF_CXXNew,
AF_CXXNewArray,
AF_IfNameIndex,
AF_Alloca,
- AF_InnerBuffer
+ AF_InnerBuffer,
+ AF_Custom,
+};
+
+class AllocationFamily {
+public:
+ AllocationFamily(AllocationFamilyKind kind,
+ std::optional<StringRef> name = std::nullopt)
+ : _kind(kind), _customName(name) {
+ assert(kind != AF_Custom || name != std::nullopt);
+ }
+
+ bool operator==(const AllocationFamily &Other) const {
+ if (Other.kind() == this->kind()) {
+ return this->kind() == AF_Custom ? this->_customName == Other._customName
+ : true;
+ } else {
+ return false;
+ }
+ }
+
+ bool operator==(const AllocationFamilyKind &kind) {
+ return this->kind() == kind;
+ }
+
+ bool operator!=(const AllocationFamilyKind &kind) { return !(*this == kind); }
+
+ std::optional<StringRef> name() const { return _customName; }
+ AllocationFamilyKind kind() const { return _kind; }
+
+private:
+ AllocationFamilyKind _kind;
+ std::optional<StringRef> _customName;
};
} // end of anonymous namespace
@@ -194,7 +226,7 @@ class RefState {
void Profile(llvm::FoldingSetNodeID &ID) const {
ID.AddInteger(K);
ID.AddPointer(S);
- ID.AddInteger(Family);
+ ID.AddInteger(Family.kind());
}
LLVM_DUMP_METHOD void dump(raw_ostream &OS) const {
@@ -1670,15 +1702,18 @@ MallocChecker::MallocMemReturnsAttr(CheckerContext &C, const CallEvent &Call,
if (!State)
return nullptr;
- if (Att->getModule()->getName() != "malloc")
- return nullptr;
+ // Preseve previous behavior when "malloc" class means AF_Malloc
+ auto attrClassName = Att->getModule()->getName();
+ auto AF = attrClassName == "malloc"
+ ? AF_Malloc
+ : AllocationFamily(AF_Custom, attrClassName);
if (!Att->args().empty()) {
return MallocMemAux(C, Call,
Call.getArgExpr(Att->args_begin()->getASTIndex()),
- UndefinedVal(), State, AF_Malloc);
+ UndefinedVal(), State, AF);
}
- return MallocMemAux(C, Call, UnknownVal(), UndefinedVal(), State, AF_Malloc);
+ return MallocMemAux(C, Call, UnknownVal(), UndefinedVal(), State, AF);
}
ProgramStateRef MallocChecker::MallocMemAux(CheckerContext &C,
@@ -1830,16 +1865,18 @@ ProgramStateRef MallocChecker::FreeMemAttr(CheckerContext &C,
if (!State)
return nullptr;
- if (Att->getModule()->getName() != "malloc")
- return nullptr;
+ // Preseve previous behavior when "malloc" class means AF_Malloc
+ auto attrClassName = Att->getModule()->getName();
+ auto AF = attrClassName == "malloc"
+ ? AF_Malloc
+ : AllocationFamily(AF_Custom, attrClassName);
bool IsKnownToBeAllocated = false;
for (const auto &Arg : Att->args()) {
- ProgramStateRef StateI =
- FreeMemAux(C, Call, State, Arg.getASTIndex(),
- Att->getOwnKind() == OwnershipAttr::Holds,
- IsKnownToBeAllocated, AF_Malloc);
+ ProgramStateRef StateI = FreeMemAux(
+ C, Call, State, Arg.getASTIndex(),
+ Att->getOwnKind() == OwnershipAttr::Holds, IsKnownToBeAllocated, AF);
if (StateI)
State = StateI;
}
@@ -1877,6 +1914,33 @@ static bool didPreviousFreeFail(ProgramStateRef State,
return false;
}
+static void printOwnershipTakesList(raw_ostream &os, CheckerContext &C,
+ const Expr *E) {
+ if (const CallExpr *CE = dyn_cast<CallExpr>(E)) {
+ const FunctionDecl *FD = CE->getDirectCallee();
+ if (!FD)
+ return;
+
+ if (FD->hasAttrs()) {
+ bool attrPrinted = false;
+
+ for (const auto *I : FD->specific_attrs<OwnershipAttr>()) {
+ OwnershipAttr::OwnershipKind OwnKind = I->getOwnKind();
+
+ if (OwnKind == OwnershipAttr::Takes) {
+ if (attrPrinted)
+ os << ", ";
+ else
+ os << ", which takes ownership of ";
+
+ os << '\'' << I->getModule()->getName() << "'";
+ attrPrinted = true;
+ }
+ }
+ }
+ }
+}
+
static bool printMemFnName(raw_ostream &os, CheckerContext &C, const Expr *E) {
if (const CallExpr *CE = dyn_cast<CallExpr>(E)) {
// FIXME: This doesn't handle indirect calls.
@@ -1918,26 +1982,54 @@ static bool printMemFnName(raw_ostream &os, CheckerContext &C, const Expr *E) {
static void printExpectedAllocName(raw_ostream &os, AllocationFamily Family) {
- switch(Family) {
- case AF_Malloc: os << "malloc()"; return;
- case AF_CXXNew: os << "'new'"; return;
- case AF_CXXNewArray: os << "'new[]'"; return;
- case AF_IfNameIndex: os << "'if_nameindex()'"; return;
- case AF_InnerBuffer: os << "container-specific allocator"; return;
- case AF_Alloca:
- case AF_None: llvm_unreachable("not a deallocation expression");
+ switch (Family.kind()) {
+ case AF_Malloc:
+ os << "malloc()";
+ return;
+ case AF_CXXNew:
+ os << "'new'";
+ return;
+ case AF_CXXNewArray:
+ os << "'new[]'";
+ return;
+ case AF_IfNameIndex:
+ os << "'if_nameindex()'";
+ return;
+ case AF_InnerBuffer:
+ os << "container-specific allocator";
+ return;
+ case AF_Custom:
+ os << Family.name().value();
+ return;
+ case AF_Alloca:
+ case AF_None:
+ llvm_unreachable("not a deallocation expression");
}
}
static void printExpectedDeallocName(raw_ostream &os, AllocationFamily Family) {
- switch(Family) {
- case AF_Malloc: os << "free()"; return;
- case AF_CXXNew: os << "'delete'"; return;
- case AF_CXXNewArray: os << "'delete[]'"; return;
- case AF_IfNameIndex: os << "'if_freenameindex()'"; return;
- case AF_InnerBuffer: os << "container-specific deallocator"; return;
- case AF_Alloca:
- case AF_None: llvm_unreachable("suspicious argument");
+ switch (Family.kind()) {
+ case AF_Malloc:
+ os << "free()";
+ return;
+ case AF_CXXNew:
+ os << "'delete'";
+ return;
+ case AF_CXXNewArray:
+ os << "'delete[]'";
+ return;
+ case AF_IfNameIndex:
+ os << "'if_freenameindex()'";
+ return;
+ case AF_InnerBuffer:
+ os << "container-specific deallocator";
+ return;
+ case AF_Custom:
+ os << "function that takes ownership of '" << Family.name().value() << "\'";
+ return;
+ case AF_Alloca:
+ case AF_None:
+ llvm_unreachable("suspicious argument");
}
}
@@ -2119,9 +2211,10 @@ MallocChecker::FreeMemAux(CheckerContext &C, const Expr *ArgExpr,
std::optional<MallocChecker::CheckKind>
MallocChecker::getCheckIfTracked(AllocationFamily Family,
bool IsALeakCheck) const {
- switch (Family) {
+ switch (Family.kind()) {
case AF_Malloc:
case AF_Alloca:
+ case AF_Custom:
case AF_IfNameIndex: {
if (ChecksEnabled[CK_MallocChecker])
return CK_MallocChecker;
@@ -2373,6 +2466,8 @@ void MallocChecker::HandleMismatchedDealloc(CheckerContext &C,
if (printMemFnName(DeallocOs, C, DeallocExpr))
os << ", not " << DeallocOs.str();
+
+ printOwnershipTakesList(os, C, DeallocExpr);
}
auto R = std::make_unique<PathSensitiveBugReport>(*BT_MismatchedDealloc,
@@ -3483,53 +3578,54 @@ PathDiagnosticPieceRef MallocBugVisitor::VisitNode(const ExplodedNode *N,
Sym, "Returned allocated memory");
} else if (isReleased(RSCurr, RSPrev, S)) {
const auto Family = RSCurr->getAllocationFamily();
- switch (Family) {
- case AF_Alloca:
- case AF_Malloc:
- case AF_CXXNew:
- case AF_CXXNewArray:
- case AF_IfNameIndex:
- Msg = "Memory is released";
+ switch (Family.kind()) {
+ case AF_Alloca:
+ case AF_Malloc:
+ case AF_Custom:
+ case AF_CXXNew:
+ case AF_CXXNewArray:
+ case AF_IfNameIndex:
+ Msg = "Memory is released";
+ StackHint = std::make_unique<StackHintGeneratorForSymbol>(
+ Sym, "Returning; memory was released");
+ break;
+ case AF_InnerBuffer: {
+ const MemRegion *ObjRegion =
+ allocation_state::getContainerObjRegion(statePrev, Sym);
+ const auto *TypedRegion = cast<TypedValueRegion>(ObjRegion);
+ QualType ObjTy = TypedRegion->getValueType();
+ OS << "Inner buffer of '" << ObjTy << "' ";
+
+ if (N->getLocation().getKind() == ProgramPoint::PostImplicitCallKind) {
+ OS << "deallocated by call to destructor";
StackHint = std::make_unique<StackHintGeneratorForSymbol>(
- Sym, "Returning; memory was released");
- break;
- case AF_InnerBuffer: {
- const MemRegion *ObjRegion =
- allocation_state::getContainerObjRegion(statePrev, Sym);
- const auto *TypedRegion = cast<TypedValueRegion>(ObjRegion);
- QualType ObjTy = TypedRegion->getValueType();
- OS << "Inner buffer of '" << ObjTy << "' ";
-
- if (N->getLocation().getKind() == ProgramPoint::PostImplicitCallKind) {
- OS << "deallocated by call to destructor";
- StackHint = std::make_unique<StackHintGeneratorForSymbol>(
- Sym, "Returning; inner buffer was deallocated");
- } else {
- OS << "reallocated by call to '";
- const Stmt *S = RSCurr->getStmt();
- if (const auto *MemCallE = dyn_cast<CXXMemberCallExpr>(S)) {
- OS << MemCallE->getMethodDecl()->getDeclName();
- } else if (const auto *OpCallE = dyn_cast<CXXOperatorCallExpr>(S)) {
- OS << OpCallE->getDirectCallee()->getDeclName();
- } else if (const auto *CallE = dyn_cast<CallExpr>(S)) {
- auto &CEMgr = BRC.getStateManager().getCallEventManager();
- CallEventRef<> Call =
- CEMgr.getSimpleCall(CallE, state, CurrentLC, {nullptr, 0});
- if (const auto *D = dyn_cast_or_null<NamedDecl>(Call->getDecl()))
- OS << D->getDeclName();
- else
- OS << "unknown";
- }
- OS << "'";
- StackHint = std::make_unique<StackHintGeneratorForSymbol>(
- Sym, "Returning; inner buffer was reallocated");
+ Sym, "Returning; inner buffer was deallocated");
+ } else {
+ OS << "reallocated by call to '";
+ const Stmt *S = RSCurr->getStmt();
+ if (const auto *MemCallE = dyn_cast<CXXMemberCallExpr>(S)) {
+ OS << MemCallE->getMethodDecl()->getDeclName();
+ } else if (const auto *OpCallE = dyn_cast<CXXOperatorCallExpr>(S)) {
+ OS << OpCallE->getDirectCallee()->getDeclName();
+ } else if (const auto *CallE = dyn_cast<CallExpr>(S)) {
+ auto &CEMgr = BRC.getStateManager().getCallEventManager();
+ CallEventRef<> Call =
+ CEMgr.getSimpleCall(CallE, state, CurrentLC, {nullptr, 0});
+ if (const auto *D = dyn_cast_or_null<NamedDecl>(Call->getDecl()))
+ OS << D->getDeclName();
+ else
+ OS << "unknown";
}
- Msg = OS.str();
- break;
+ OS << "'";
+ StackHint = std::make_unique<StackHintGeneratorForSymbol>(
+ Sym, "Returning; inner buffer was reallocated");
}
+ Msg = OS.str();
+ break;
+ }
case AF_None:
llvm_unreachable("Unhandled allocation family!");
- }
+ }
// See if we're releasing memory while inlining a destructor
// (or one of its callees). This turns on various common
diff --git a/clang/test/Analysis/MismatchedDeallocator-checker-test.mm b/clang/test/Analysis/MismatchedDeallocator-checker-test.mm
index 013d677e515cf..ef87951c0131e 100644
--- a/clang/test/Analysis/MismatchedDeallocator-checker-test.mm
+++ b/clang/test/Analysis/MismatchedDeallocator-checker-test.mm
@@ -14,6 +14,13 @@
void free(void *);
void __attribute((ownership_takes(malloc, 1))) my_free(void *);
+void __attribute((ownership_returns(malloc1))) *my_malloc1(size_t);
+void __attribute((ownership_takes(malloc1, 1))) my_free1(void *);
+
+void __attribute((ownership_returns(malloc2))) *my_malloc2(size_t);
+void __attribute((ownership_returns(malloc2))) *my_malloc3(size_t);
+void __attribute((ownership_takes(malloc2, 1))) __attribute((ownership_takes(malloc3, 1))) my_free23(void *);
+
//---------------------------------------------------------------
// Test if an allocation function matches deallocation function
//---------------------------------------------------------------
@@ -60,6 +67,41 @@ void testMalloc8() {
operator delete[](p); // expected-warning{{Memory allocated by malloc() should be deallocated by free(), not operator delete[]}}
}
+void testMalloc9() {
+ int *p = (int *)my_malloc(sizeof(int));
+ my_free(p); // no warning
+}
+
+void testMalloc10() {
+ int *p = (int *)my_malloc1(sizeof(int));
+ my_free1(p); // no warning
+}
+
+void testMalloc11() {
+ int *p = (int *)my_malloc2(sizeof(int));
+ my_free23(p); // no warning
+}
+
+void testMalloc12() {
+ int *p = (int *)my_malloc1(sizeof(int));
+ my_free(p); // expected-warning{{Memory allocated by my_malloc1() should be deallocated by function that takes ownership of 'malloc1', not my_free(), which takes ownership of 'malloc'}}
+}
+
+void testMalloc13() {
+ int *p = (int *)my_malloc2(sizeof(int));
+ my_free1(p); // expected-warning{{Memory allocated by my_malloc2() should be deallocated by function that takes ownership of 'malloc2', not my_free1(), which takes ownership of 'malloc1'}}
+}
+
+void testMalloc14() {
+ int *p = (int *)my_malloc1(sizeof(int));
+ my_free23(p); // expected-warning{{Memory allocated by my_malloc1() should be deallocated by function that takes ownership of 'malloc1', not my_free23(), which takes ownership of 'malloc2', 'malloc3'}}
+}
+
+void testMalloc15() {
+ int *p = (int *)my_malloc1(sizeof(int));
+ free(p); // expected-warning{{Memory allocated by my_malloc1() should be deallocated by function that takes ownership of 'malloc1', not free()}}
+}
+
void testAlloca() {
int *p = (int *)__builtin_alloca(sizeof(int));
delete p; // expected-warning{{Memory allocated by alloca() should not be deallocated}}
|
|
Hello @steakhal, could you, please, take a look? I just found linked issue quite interesting and decided to give it a try. I am new to llvm world, so I would really appreciate your review |
There was a problem hiding this comment.
Looks pretty good. Thanks for working on the issue.
I didn't see any major blocking issue with the PR, so good job on that!
Overall, be sure to follow the llvm coding style, including using upper case variable names.
Thanks again for working on this :)
llvmbot
added
the
clang:frontend
There was a problem hiding this comment.
It looks great! I only had one more thing to test.
Besides that it would be nice to advertise this in the clang/docs/ReleaseNotes.rst.
Usually, from that release notes we also link to the checker or the relevant doc pages - in this case it would be nice to link to the docs of this attribute - which does not really elaborate on the attribute and the semantics.
Do you want to extend the scope of this PR to add some minimal docs to the attribute?
If not, that's also fine, we will create a separate ticket for adding them later.
|
|
||
| int f19(void *) | ||
| __attribute__((ownership_takes(foo, 1))) // expected-error {{'ownership_takes' attribute class does not match; here it is 'foo'}} | ||
| __attribute__((ownership_takes(foo1, 1))); // expected-note {{declared with class 'foo1' here}} |
There was a problem hiding this comment.
What happens if I want to redeclare the function with a different "ownership_takes" attribute in a subsequent line? Will it detect the conflict?
int f20(void *) __attribute__((ownership_takes(foo, 1)));
int f20(void *) __attribute__((ownership_takes(foo1, 1))); // redecl with different attr
There was a problem hiding this comment.
Hmmmmm.... It does not detect. As well as for other attributes.
I think, we should fix it, but maybe all of them in scope of separate PR?
I would be happy to give it a try, but I think we need to fix couple of things to truly follow the semantics. As you have already noticed, attributes conflicts on different declarations are not reported. Also I found out that code like does not cause an error. I will fill an issue for these cases. Also, thank you so much for review! |
There was a problem hiding this comment.
I think it looks great.
Only a few more stylistic remarks are left, but it should be already in a good shape functional-wise. I'll let you decide if you wanna apply my suggestions or not.
One more remark - this time w.r.t. the workflow. Usually, GitHub PRs prefer "merges" over "force-pushes". Whenever you force-push, all the inline remarks could get lost, as it fails to track the lines where the comments were made. Thus, when working with PRs, one usually just "merges" the tracked branch, and just builds more and more commits on top of your PR branch. Once the review is done, one is free to destroy the history and squash the commits, or do some reordering - because there are no more pending comments or remarks left.
|
Thank you for review! I've applied your suggestions and fixed test failures. Messed up a bit with shadowing in constructor.
Oh, ok. I've never really worked with gh workflow, so didn't know that. Thank you for guidance! |
steakhal
changed the title
ownership_{returns,takes} attributes
|
FYI This PR is still not mentioned in the
|
I can't find any docs for this attribute. As I mentioned, I will fill new issues to fix couple of frontend issues and after that we can write down correct semantics of these attrs. Do you mean that link https://clang.llvm.org/docs/AttributeReference.html#ownership-holds-ownership-returns-ownership-takes? |
Thats fine.
Yes. Its generated from a tablegen file. Thats why you probably didnt find it. |
|
Added release note and added basic example to Thanks! I learned a lot about llvm workflow. (Ah, again force pushed for some reason, sorry) |
Add support for custom allocation classes that could be specified by ownership_returns attribute. This patch adds basic support, so new class is not contructed anywhere and handled as an error everywhere.
Patch introduces support for custom allocation classes in MismatchedDeallocator. Patch preserves previous behavior, in which 'malloc' class was handled as AF_Malloc type.
|
Rebased on top of d89f3e8 |
|
Should I merge this PR, or you want to wait some? |
|
I think, it's good time to merge, since v19 release branch was created. If you want to wait for others to review, it's also fine. |
|
@pskrgag Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested Please check whether problems have been caused by your change specifically, as How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
) Summary: Add support for checking mismatched ownership_returns/ownership_takes attributes. Closes #76861 Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60250804
…m#98941) Add support for checking mismatched ownership_returns/ownership_takes attributes. Closes llvm#76861
Add support for checking mismatched ownership_returns/ownership_takes attributes.
Closes #76861